Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow to use ansible-test's change detection for Pull Requests #49

Merged
merged 2 commits into from
Dec 3, 2022
Merged

Allow to use ansible-test's change detection for Pull Requests #49

merged 2 commits into from
Dec 3, 2022

Conversation

felixfontein
Copy link
Contributor

Re-opening #46. I rebased with latest main and squashed some commits.

@felixfontein felixfontein mentioned this pull request Nov 14, 2022
Merged
@felixfontein
Copy link
Contributor Author

Also besides that, I really don't want any code coverage to be uploaded for PRs in the collections where I plan to use this.

I wonder why. Is that because you think that their bot comments/statuses would be annoying? I think that could be disabled, and it'd be made silent.

It's because the AZP scripts don't do this either, so I don't see why I want to have some of the CI runs to send coverage data and the others (which will be the majority) none.

Also, I can imagine that some people would still want this, so maybe it's reasonable to have a flag/input for this?

That's what I'm planning to implement anyway. I want to be able to turn off code coverage reporting completely. (Not being able to do so prevented me using this action in a role only collection, as with stable-2.9 the action errors out if there are no coverage reports to upload.)

@felixfontein
Copy link
Contributor Author

Failing CI is unrelated...

@webknjaz
Copy link
Member

It's because the AZP scripts don't do this either, so I don't see why I want to have some of the CI runs to send coverage data and the others (which will be the majority) none.

Looks like this wouldn't apply to the repos that don't use AZP at all. Note that among this action, users are projects outside the @ansible-collections community. I'd very much want them to be able to take advantage of the practices that are common among more generic software projects on GitHub, not limiting them with what the Ansible Community does currently.

@webknjaz
Copy link
Member

Not being able to do so prevented me using this action in a role only collection, as with stable-2.9 the action errors out if there are no coverage reports to upload.

What exactly fails? Can this be fixed in the action?

webknjaz
webknjaz reviewed Nov 14, 2022
View reviewed changes
action.yml Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Nov 14, 2022
View reviewed changes
action.yml Show resolved Hide resolved
webknjaz
webknjaz reviewed Nov 14, 2022
View reviewed changes
action.yml Outdated Show resolved Hide resolved
@felixfontein
Copy link
Contributor Author

Not being able to do so prevented me using this action in a role only collection, as with stable-2.9 the action errors out if there are no coverage reports to upload.

What exactly fails? Can this be fixed in the action?

See https://github.com/felixfontein/ansible-acme/actions/runs/3315379162/jobs/5475899754

@felixfontein
Copy link
Contributor Author

It's because the AZP scripts don't do this either, so I don't see why I want to have some of the CI runs to send coverage data and the others (which will be the majority) none.

Looks like this wouldn't apply to the repos that don't use AZP at all. Note that among this action, users are projects outside the @ansible-collections community. I'd very much want them to be able to take advantage of the practices that are common among more generic software projects on GitHub, not limiting them with what the Ansible Community does currently.

As I said, I don't mind making code coverage configurable in the sense that you can say "always enable", "always disable", or "only enable when no change detection is used" (i.e. as in this PR). I still don't think that these carryforward flags will work and I don't want to implement that though. And also I don't think this extra configurability should happen in this PR.

webknjaz
webknjaz reviewed Nov 15, 2022
View reviewed changes
action.yml Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Nov 15, 2022
View reviewed changes
action.yml Show resolved Hide resolved
webknjaz
webknjaz reviewed Nov 15, 2022
View reviewed changes
action.yml Show resolved Hide resolved
webknjaz
webknjaz reviewed Nov 15, 2022
View reviewed changes
action.yml Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Nov 15, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
This was referenced Nov 27, 2022
Merged
Merged
Merged
Merged
@felixfontein @webknjaz
Implement opt-in change detection in pull requests
b89e4e4 
This patch introduces a `pull-request-change-detection` action input
that allows the end-users to request running `ansible-test` in change
detection mode. It is only supported in for pull requests at the
moment because `ansible-test` does not currently know how to interact
with the environment variables that GitHub Actions CI/CD workflows
set [1].

With this feature, the users will be able to save some of the CI
compute time due to `ansible-test` skipping the tests that are not
affected by the changes in PRs.

[1] https://github.com/ansible-community/ansible-test-gh-action/pull/49/files#r1023412786
@felixfontein @webknjaz
Add smoke/integration tests for change detection
5257e71
@webknjaz webknjaz merged commit 1c6026e into ansible-community:main Dec 3, 2022
@webknjaz
Copy link
Member

webknjaz commented Dec 3, 2022

@felixfontein thanks for the patch! I've cleaned it up a bit and released as v1.12.0. Have fun with it!

@felixfontein felixfontein deleted the change-detection branch December 3, 2022 12:32
@felixfontein
Copy link
Contributor Author

@webknjaz thanks for reviewing, cleaning up, and merging!

@Andersson007 Andersson007 mentioned this pull request Dec 9, 2022
Merged
This was referenced Dec 9, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattclay mattclay mattclay left review comments

@webknjaz webknjaz Awaiting requested review from webknjaz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@felixfontein @webknjaz @mattclay